Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.69%. Comparing base (a75818a) to head (6795ff4).

@@           Coverage Diff           @@
##           master    #4170   +/-   ##
=======================================
  Coverage   93.69%   93.69%           
=======================================
  Files         210      210           
  Lines       19763    19763           
=======================================
  Hits        18517    18517           
  Misses       1049     1049           
  Partials      197      197           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thank you, @alexandear - my only concern with this change is that it now makes it more difficult to write the unit tests, as you need to know what the body is going to look like instead of allowing the test harnest ensure that what went in actually came out.

How about we make a helper function like :

func testBodyAsJSON[T any](t *testing.T, r *http.Request, want T) {
	// Do json unmarshal of the request body and compare the result with the expected struct.
}

With this approach, we can pass the same input that was provided to the method.
But it will still have complexity of assertNilError.

@alexandear - what are your thoughts about @Not-Dhananjay-Mishra's idea above?

I really like the idea of the generics helper so that users don't have to manually write the JSON version of their data (or have to first run the failing test and copy/paste the actual JSON blob back into the code), as it also cuts down on redundant data.

I split testBody into testJSONBody and testPlainBody, and removed the redundant \n.

it now makes it more difficult to write the unit tests, as you need to know what the body is going to look like instead of allowing the test harnest ensure that what went in actually came out.

We already have around 100 unit tests that use testBody, so this approach is consistent with the existing test suite.

In practice, the exact body string can be copied directly into a test from the GitHub documentation (the -d field in the curl example), which keeps the process straightforward. For example,

Regarding the @Not-Dhananjay-Mishra suggestion about testBodyAsJSON: I believe we should prefer raw strings rather than relying on encoding/json marshaling in tests. This would allow us to catch potential issues earlier, especially if we migrate to encoding/json/v2. In essence, raw strings are what are sent over the wire.

In practice, the exact body string can be copied directly into a test from the GitHub documentation (the -d field in the curl example), which keeps the process straightforward. For example,

I'm not convinced that this is any easier because even if the developer copy/pastes the raw JSON from the documentation, they would still have to manually come up with the Go equivalent that generated that raw JSON.

I believe we should prefer raw strings rather than relying on encoding/json marshaling in tests. This would allow us to catch potential issues earlier, especially if we migrate to encoding/json/v2. In essence, raw strings are what are sent over the wire.

So to me, the point of this testing is to make sure that the structs are correct and that all the fields have the correct tags in order to properly send the data over the wire and be successfully decoded on the other side. The developer is going to have to generate an example in order to test this. However, with this PR, the developer will have to generate two copies of this data in two different formats (Go structs and raw JSON), and they MUST match or the tests will fail.

If, however, we use generics as @Not-Dhananjay-Mishra suggests, the developer will only have to come up with the Go structs format of the example, and the machinery will handle the rest to make sure that the code can properly generate the over-the-wire raw JSON string, then parse it back, decode it, and make sure they match.

It seems to me that this latter method is a far superior developer experience without any downsides.

Or am I missing something?

Perhaps we should allow @Not-Dhananjay-Mishra to come up with an alternative implementation of this PR that addresses the identical concerns but uses the generics helper and we can compare the two PRs in terms of developer experience?

Or am I missing something?

Your point is valid. Thank you for the explanation.

Perhaps we should allow @Not-Dhananjay-Mishra to come up with an alternative implementation of this PR that addresses the identical concerns but uses the generics helper and we can compare the two PRs in terms of developer experience?

Agree. Let's implement an alternative version to compare against this PR.

Perhaps we should allow @Not-Dhananjay-Mishra to come up with an alternative implementation of this PR that addresses the identical concerns but uses the generics helper and we can compare the two PRs in terms of developer experience?

Raised #4183